Added implementation of filter refraction as a photon operator#103
Added implementation of filter refraction as a photon operator#103FedericoBerlfein wants to merge 8 commits into
Conversation
sidneymau
left a comment
There was a problem hiding this comment.
Left a few comments. The math throughout looks reasonable, but I'd have to dedicate a bit more time to fully review everything here
|
|
||
|
|
||
| # =========================================================================== | ||
| # Roman WFI instrument constants |
There was a problem hiding this comment.
is there a global reference these can come from? Not sure if this info exists already in romanisim or somewhere else? (cc @yuedong0607)
There was a problem hiding this comment.
For most of them I can point to this documentation for reference. For the pupil demagnification I don't think there is a public reference. The pupil is not perfectly circular, and the demagnification can vary across the FOV. What I did initially was simply calculate the average across the FOV along the x-direction and use that. But to be more exact here, I updated the code to use the slightly more exact version, which uses a different demagnification for the x- and y-axis. I added some documentation in the constants that hopefully makes things clear.
There was a problem hiding this comment.
thanks for clarifying -- and I agree that it's good to use the slightly more exact version, especially if anyone in the future assumes that it has been implemented and is doing a study sensitive to the difference.
My original comment had more to do with, for example, the pixel size, scale, focal ratio, diameter, etc. -- I would imagine that there is some common reference for all of these values elsewhere in galsim/roman_imsim/romanisim, but I'm not sure where the base truth should live. If you think it's better to define these all locally and not use something else, that's fine, but I do think it's worth consideration
There was a problem hiding this comment.
Got it that makes sense. For the pixel size, scale, and diameter, I considered importing them from galsim.roman, but it felt odd to have 3 constants exported from there and everything else hard-coded, so I opted to keep everything defined locally. I could not find these constants defined in roman_imsim because I think the code just calls galsim.roman directly when it needs them.
There was a problem hiding this comment.
@yuedong0607 probably knows best where these constants are defined most generally?
|
@sidneymau thank you for the comments! I think I addressed all of them above. I did a small update to the pupil demagnification, mostly to upgrade to a slightly more accurate version. Before I was using a single factor (for both x- and y-axis), but technically the demagnification can can be different in the x/y direction because the pupil is not perfectly circular. This affects the angle of incidence in each direction, but the change is truly quite minimal and does not really affect the effect in practice. I just thought to include it for completeness. |
sidneymau
left a comment
There was a problem hiding this comment.
Thanks for addressing to my earlier comments! Before approving, I think there are a few items to do:
- make sure our reference image action still passes and/or update appropriately (cc @arunkannawadi for thoughts on if this refraction should be part of the default configuration here)
- we should probably write some tests for all of the functions throughout
|
Regarding the tests, I can get started on that. I see that in the |
|
Yeah, the lack of unit tests is a big TODO for this project. I think a new file specifically for the photon operator -- or even for the filter refraction -- tests would be a good first step towards this. |
Implementation of refraction from the WFI filters as a photon operator. The photon operator lives in the file
filter_refraction.pyand contains the relevant functions used in the photon operator. Modifications to__init__.pyare simply to add the new file and functions within it. Modifications toconfig/default.yamlare to add refraction prior to charge diffusion. I believe this is the correct order, as this effect occurs before photons reach the detector.A small breakdown of the functions in
filter_refraction.py:class RomanFilterRefraction: This class contains the GalSim photon operator. To initialize the operator, you need to specify the bandpass, SCA and SCA position, pixel scale, and callable function to get the index of refraction as a function of wavelength. TheapplyTogets the lateral chromatic shifts induced by filter refraction and applies it directly to the photons based on their wavelength. The default for the pixel scale is the native roman scale (0.11 arcsec/pixel), and the default index of refraction comes from the Roman WFI filter substrate (Suprasil 3001)class RomanFilterRefractionBuilder: This class builds the photon operator from the information inbase, which includes the bandpass, SCA and SCA position._get_lateral_shifts: This is the main function called withinapplyTothat calculates the lateral chromatic shifts. This function calls other helper functions defined throughout the file.pysiaf)